Slicer update (C++ and bindings) - #67
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9210a71e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (nanobind::isinstance<nanobind::iterable>(obj)) { | ||
| auto it = nanobind::cast<nanobind::iterable>(obj); | ||
| auto begin = it.begin(); | ||
| if (begin != it.end()) return _get_dtype(*begin); | ||
| return Array_dtype::EMPTY; |
There was a problem hiding this comment.
When an advertised iterable argument is a generator or iterator, calling begin() and dereferencing it here consumes its first item. The subsequent conversion of filtration_values therefore sees a missing first row (and nested one-shot row iterators similarly lose their first coordinate), causing shape failures or shifted filtration data; dtype inspection must either buffer the item or avoid iterating one-shot inputs twice.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is not really meant for iterables with one-shot iterators, but there is no satisfying way to rule them out. But as it will probably only be used by lists, tuples or tensors, I added the condition of existing __getitem__ and __len__ to be able to add a guard. It is just not satisfying. The real thing I would like to test here is "is it a range of something" and "can I have access to the first element several times", but I could not find a solution which always works for that...
And copying the container to ensure several accesses is out of question, as the containers can be huge. In particular as one-shot iterables are very unlikely to be ever used here.
2 participants
I went through the whole Slicer bindings to add the functionalities missing in the backend C++ Slicer. At the same occasion I reorganized the bindings, here a few comments about it:
_slicer_nanobind.cppis now decomposed in three file (2 headers). This is meant to be temporary. It helped me organizing the different binding types done in this file and at the end to separate what I modified and what not. There will be a follow up PR where I unify everything again.SlicerConversionanymore or perhaps because of something else. We should think of a solution to avoid binding that many Slicers in the first place (perhaps by binding a single virtual Slicer instead and have a pointer factory to instantiate them with particular Slicers?). We could also compile theSlicer_interfaceclasses separately as you did withSlicerConversionandSlicerto help parallelization.